Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🐛 [Story Preview] Remove Android poster image from playing videos in preview mode #38291

Merged
merged 9 commits into from
Jul 26, 2022
Merged

🐛 [Story Preview] Remove Android poster image from playing videos in preview mode #38291

merged 9 commits into from
Jul 26, 2022

Conversation

coreymasanto
Copy link
Contributor

@coreymasanto coreymasanto commented Jun 13, 2022 •
edited
Loading

Background: Android sometimes shows a blank frame between the poster and the first frame, which is an issue that was resolved by rendering the poster image while the first frame buffers.

Issue: Although the poster image is removed once the video is loaded, this removal occurs in firstLayoutCompleted(), which is only called once layoutCallback() has resolved. The issue is that layoutCallback() cannot resolve in the preview state because of a blocking whenFirstVisible() call that resolves only once the document progresses to the visible state.

Fix: This PR removes the poster image in onVideoLoaded_() (which is already being called from the layoutCallback()), instead of removing it only after the entire layoutCallback() has resolved (i.e., via firstLayoutCompleted()).

@coreymasanto coreymasanto self-assigned this Jun 13, 2022
@coreymasanto coreymasanto marked this pull request as ready for review June 13, 2022 19:04
@amp-owners-bot amp-owners-bot bot requested a review from dmanek June 13, 2022 19:04
@coreymasanto coreymasanto changed the title 🐛 [Story Preview] Remove Android poster image from playing preview videos 🐛 [Story Preview] Remove Android poster image from playing videos in preview mode Jun 13, 2022
@coreymasanto coreymasanto requested review from alanorozco and removed request for dmanek and mszylkowski June 13, 2022 19:08
mszylkowski
mszylkowski reviewed Jun 14, 2022
View reviewed changes
Copy link
Contributor

@mszylkowski mszylkowski left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test on Android if this makes the background flash between the placeholder and the video frames? There are some examples on #31358 of this behavior. As long as it doesn't make the issue worse, it should be fine.

@coreymasanto
Copy link
Contributor Author

Did you test on Android if this makes the background flash between the placeholder and the video frames? There are some examples on #31358 of this behavior. As long as it doesn't make the issue worse, it should be fine.

Tested loading a story in the visible state on Android. On this PR vs at main, I don't see any background flash between the grey city placeholder image and the subsequent video frames.

This PR:

screen-20220621-131550_2.mp4

Main:

screen-20220621-131811_2.mp4

@alanorozco
Copy link
Member

@coreymasanto

Did you try 1) no workaround on 2) a throttled network?

We might not need the workaround anymore.

@coreymasanto
Copy link
Contributor Author

Did you try 1) no workaround on 2) a throttled network?

We might not need the workaround anymore.

When I comment out the this.createPosterForAndroidBug_() workaround and test on various connection speeds at both main and this PR, I still don't see any background flash between the poster image and the first video frame. I'm not confident about removing that method in this PR, though.

@coreymasanto coreymasanto enabled auto-merge (squash) July 18, 2022 16:38
@coreymasanto coreymasanto merged commit b73520d into ampproject:main Jul 26, 2022
@coreymasanto coreymasanto deleted the previewAndroidPoster branch July 26, 2022 17:25
@ampprojectbot
Copy link
Member

Warning: disparity between this PR Percy build and its main build

The Percy build for this PR was approved (either manually by a member of the AMP team, or automatically if there were no visual diffs). However, during a continuous integration step we generated another Percy build using the commit on the main branch that this PR was merged into, and there appears to be a mismatch between the two.

This is possibly an indication of an issue with this pull request, but could also be the result of flakiness. Please inspect the two builds < This PR's Percy build / main commit's Percy build > and determine further action:

  • If the disparity appears to be caused by this PR, please create an bug report or send out a new PR to fix
  • If the disparity appears to be a flake, please @-mention ampproject/wg-approvers in a comment
  • If there is no disparity and this comment was created by mistake, please @-mention ampproject/wg-infra
  • If unsure, @-mention ampproject/wg-approvers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

@alanorozco alanorozco Awaiting requested review from alanorozco

@mszylkowski mszylkowski Awaiting requested review from mszylkowski

Assignees

@coreymasanto coreymasanto

Labels
PR use: In Beta / Experimental PR use: In LTS PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@coreymasanto @alanorozco @ampprojectbot @gmajoulet @mszylkowski